Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Javadocs] add remaining internal classes and reenable missingJavadoc on server #3296

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented May 12, 2022

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module. From here forward if class level javadocs are
missing in the server module, gradle check will fail!

relates #221
relates #2868

@nknize nknize added documentation Improvements or additions to documentation non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch labels May 12, 2022
@nknize nknize requested review from a team and reta as code owners May 12, 2022 05:18
@nknize nknize force-pushed the javadocs/server/internalClasses/remainingAndReenableCheckForServer branch from 645ced6 to ce83ed4 Compare May 12, 2022 05:19
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure ce83ed489f4c91184e8822a0c4c726319e71ca7c
Log 5246

Reports 5246

nknize added 2 commits May 12, 2022 07:41
… on server

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Nicholas Walter Knize <[email protected]>
@nknize nknize force-pushed the javadocs/server/internalClasses/remainingAndReenableCheckForServer branch from ce83ed4 to 3d97ca1 Compare May 12, 2022 12:50
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 3d97ca1
Log 5248

Reports 5248

Signed-off-by: Nicholas Walter Knize <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 805305c
Log 5250

Reports 5250

Copy link
Member

@andrross andrross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Just curious, do we have any tooling (or plans for tooling) that enforce that @opensearch.internal annotated classes aren't used externally?

@nknize nknize merged commit 3aef125 into opensearch-project:main May 12, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3296-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3aef125d0dbf2f29d9bf00b84cb52892a27a5328
# Push it to GitHub
git push --set-upstream origin backport/backport-3296-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3296-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.0 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.0 2.0
# Navigate to the new working tree
cd .worktrees/backport-2.0
# Create a new branch
git switch --create backport/backport-3296-to-2.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3aef125d0dbf2f29d9bf00b84cb52892a27a5328
# Push it to GitHub
git push --set-upstream origin backport/backport-3296-to-2.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.0

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport/backport-3296-to-2.0.

@nknize
Copy link
Collaborator Author

nknize commented May 12, 2022

do we have any tooling (or plans for tooling) that enforce that @opensearch.internal annotated classes aren't used externally?

No... (not yet). For now the only enforcement would be to follow up and refactor any sloppily used modifiers from public to package private or private. I was curious if java annotation enforcement could be a job for the forbiddenAPI? I'm not that familiar with using forbiddenAPIs and if this is the right application of it's services. Maybe java policeman @uschindler has some thoughts?

@uschindler
Copy link
Contributor

uschindler commented May 12, 2022

Hi,
You would need some classfile scanner that produces a forbiddenapis signatures file. For @Deprecated java annotation there is a processor available as Java class with a complementing Groovy script in forbiddenapis to be adapted (replace annotation name and tell it which jar files to scan).

https://github.com/policeman-tools/forbidden-apis/blob/main/src/tools/java/de/thetaphi/forbiddenapis/DeprecatedGen.java

So one could generate signatures files automatically after compile.

The question is more: why would anybody use it in downstream code?

@nknize
Copy link
Collaborator Author

nknize commented May 12, 2022

The question is more: why would anybody use it in downstream code?

I'm thinking of it being used in an opensearch plugin specific context. Something like K-NN plugin which now injects a custom codec - whereas before it was overriding the entire engine. There are certain classes we just don't want to be able to override in a downstream plugin. We're getting better about fixing the class modifiers (Elastic never worried about this since it was never a community project) but we have a long way to go. In the meantime I've started liberally applying the @opensearch.internal javadoc annotation to communicate that plugins should not override anything with that annotation.

I wonder if we could enforce this in build-tools without breaking the core build? Build the signature file when build-tools is built and have the downstream plugins consume it during their plugin build?

@nknize nknize added backport 2.x Backport to 2.x branch and removed backport 2.x Backport to 2.x branch labels May 13, 2022
nknize added a commit that referenced this pull request May 13, 2022
… on server (#3296)

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module. From here forward if class level javadocs are
missing in the server module, gradle check will fail!

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 3aef125)
nknize pushed a commit that referenced this pull request May 13, 2022
… missingJavadoc on server (#3296) (#3318)

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module. From here forward if class level javadocs are
missing in the server module, gradle check will fail!

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 3aef125)
nknize added a commit to nknize/OpenSearch that referenced this pull request May 14, 2022
… on server (opensearch-project#3296)

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module. From here forward if class level javadocs are
missing in the server module, gradle check will fail!

Signed-off-by: Nicholas Walter Knize <[email protected]>
nknize added a commit that referenced this pull request May 14, 2022
… missingJavadoc on server (#3296) (#3319)

Adds the remaining javadocs to internal classes and reenables the missingJavadoc
gradle task on the server module. From here forward if class level javadocs are
missing in the server module, gradle check will fail!

Signed-off-by: Nicholas Walter Knize <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch documentation Improvements or additions to documentation non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants